Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve graph visualization documentation #504

Closed
wants to merge 2 commits into from

Conversation

NivekT
Copy link
Contributor

@NivekT NivekT commented Jun 9, 2022

Stack from ghstack:

Improve docstring, added an example, and expanded documentation to have a template for utility functions (not just class)

If there isn't strong objection, I plan to cherry-pick this and the graph visualization PR to release 0.4.0 (cc: @ejguan).

Differential Revision: D37040783

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 9, 2022
@NivekT NivekT requested a review from ejguan June 9, 2022 15:45
@NivekT
Copy link
Contributor Author

NivekT commented Jun 9, 2022

@NivekT has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and I am fine to cherry pick the graph visualization PRs to the release.

I am still a noob regarding these rst files. Could you please provide a reference of rendered page?

Improve docstring, added an example, and expanded documentation to have a template for utility functions (not just class)

If there isn't strong objection, I plan to cherry-pick this and the graph visualization PR to release 0.4.0 (cc: ejguan).

[ghstack-poisoned]
@NivekT
Copy link
Contributor Author

NivekT commented Jun 9, 2022

@NivekT has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@NivekT
Copy link
Contributor Author

NivekT commented Jun 9, 2022

LGTM, and I am fine to cherry pick the graph visualization PRs to the release.

I am still a noob regarding these rst files. Could you please provide a reference of rendered page?

Here you go:

Screen Shot 2022-06-09 at 1 23 35 PM

Screen Shot 2022-06-09 at 1 23 21 PM

@ejguan
Copy link
Contributor

ejguan commented Jun 9, 2022

Thank you! Looks great.

@NivekT NivekT mentioned this pull request Jun 9, 2022
NivekT added a commit that referenced this pull request Jun 9, 2022
Summary:
Pull Request resolved: #504

Improve docstring, added an example, and expanded documentation to have a template for utility functions (not just class)

If there isn't strong objection, I plan to cherry-pick this and the graph visualization PR to release 0.4.0 (cc: ejguan).

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D37040783

Pulled By: NivekT

fbshipit-source-id: 80b9569ad5a26b5a03e02d38001f88ec0599bc4b
NivekT added a commit that referenced this pull request Jun 9, 2022
Summary:
Pull Request resolved: #504

Improve docstring, added an example, and expanded documentation to have a template for utility functions (not just class)

If there isn't strong objection, I plan to cherry-pick this and the graph visualization PR to release 0.4.0 (cc: ejguan).

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D37040783

Pulled By: NivekT

fbshipit-source-id: 80b9569ad5a26b5a03e02d38001f88ec0599bc4b
@facebook-github-bot facebook-github-bot deleted the gh/NivekT/81/head branch June 13, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants